Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Yabeda integration for prom metrics #1241

Merged
merged 16 commits into from
Nov 7, 2024
Merged

Yabeda integration for prom metrics #1241

merged 16 commits into from
Nov 7, 2024

Conversation

ngetahun
Copy link
Contributor

@ngetahun ngetahun commented Oct 29, 2024

Description

How to test

Start the rails server:
> bin/rails s
On a separate console, run:
> curl localhost:4224/metrics

Should see a text with metrics and values

Change Type

Please select the correct option.

  • Bug Fix (a non-breaking change which fixes an issue)
  • New Feature (a non-breaking change which adds new functionality)
  • Documentation Update (a change which only updates documentation)

Checklist

Please check off each item if the requirement is met.

  • I have reviewed my own code and believe that it's ready for an external review.
  • I have provided comments for any hard-to-understand code.
  • I have documented the MANUAL.md file with any changes to the user experience.
  • If my changes are non-trivial, I have added a changelog entry to notify users at package/obs/rmt-server.changes.

Review

Please check out our review guidelines
and get in touch with the author to get a shared understanding of the change.

@ngetahun ngetahun added enhancement Usability improvements or new feature requests. WIP Work in progress, do not merge. labels Oct 29, 2024
@ngetahun ngetahun marked this pull request as draft October 29, 2024 07:17
Copy link
Contributor

@josegomezr josegomezr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're almost there! Left you some comments to trim a bit what's not used in RMT.

@ngetahun ngetahun marked this pull request as ready for review October 29, 2024 16:23
@ngetahun ngetahun changed the title WIP: Test yabeda integration for prom metrics Test yabeda integration for prom metrics Oct 29, 2024
@ngetahun ngetahun removed the WIP Work in progress, do not merge. label Oct 29, 2024
@ngetahun ngetahun changed the title Test yabeda integration for prom metrics Yabeda integration for prom metrics Oct 29, 2024
Copy link
Contributor

@felixsch felixsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

I'm not really sure what is the scope. The metric export now most information, I hope this is fine.

How I reviewed this merge request:

  • Checked the code changes make sense to me
  • Ran the branch and checked /metrics is populated (also in error case)

As always, if you think I missed something I should test, please let me know!🚀

cheers,

Error gets built automagically by rspec,
no need to { error: <string> }.to_json
Expect was too granular and specific
@ngetahun
Copy link
Contributor Author

ngetahun commented Nov 5, 2024

@ngetahun ngetahun force-pushed the test_yabeda branch 2 times, most recently from 50df6dc to 384b00b Compare November 7, 2024 10:40
@ngetahun ngetahun merged commit ac5c206 into master Nov 7, 2024
3 checks passed
@ngetahun ngetahun deleted the test_yabeda branch November 7, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Usability improvements or new feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants